Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added .not() matcher #81

Closed
wants to merge 15 commits into from
Closed

Added .not() matcher #81

wants to merge 15 commits into from

Conversation

cakeinpanic
Copy link
Contributor

@cakeinpanic cakeinpanic commented Jan 23, 2018

I've added .not() method for matchers

There are two options: one looks not so great as in jasmine, but still offers some opportunities:

verify(mockedFoo.getBar(stricktEqual(2).not())).once();
verify(mockedFoo.getBar(anyNumber.not()).times(4); 

Another one looks more alike, but I have some doubts about architecture

verify(mockedFoo.getBar(not().stricktEqual(2)))).once();
verify(mockedFoo.getBar(not().anyNumber)).times(4); 

What do you think?

@cakeinpanic cakeinpanic changed the title Not Added .not() matcher Jan 23, 2018
@codecov-io
Copy link

codecov-io commented Jan 23, 2018

Codecov Report

Merging #81 into master will increase coverage by 0.75%.
The diff coverage is 87.34%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #81      +/-   ##
==========================================
+ Coverage   94.27%   95.02%   +0.75%     
==========================================
  Files          33       34       +1     
  Lines         576      623      +47     
  Branches       67       69       +2     
==========================================
+ Hits          543      592      +49     
+ Misses         24       22       -2     
  Partials        9        9
Impacted Files Coverage Δ
src/utils/MockableFunctionsFinder.ts 100% <ø> (ø) ⬆️
src/matcher/type/Not.ts 100% <100%> (ø)
src/matcher/type/AnyFunctionMatcher.ts 100% <100%> (ø) ⬆️
src/matcher/type/AnyOfClassMatcher.ts 100% <100%> (ø) ⬆️
src/ts-mockito.ts 96.92% <100%> (+0.14%) ⬆️
src/matcher/type/Matcher.ts 100% <100%> (+33.33%) ⬆️
src/matcher/type/AnyNumberMatcher.ts 88.88% <50%> (ø) ⬆️
src/matcher/type/AnyStringMatcher.ts 88.88% <50%> (ø) ⬆️
src/matcher/type/NotNullMatcher.ts 87.5% <50%> (ø) ⬆️
src/matcher/type/AnythingMatcher.ts 87.5% <50%> (ø) ⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 67c87ab...b581f47. Read the comment docs.

@NagRock
Copy link
Owner

NagRock commented Jan 24, 2018

@cakeinpanic wow another PR :)

I think that second version is more human readable not().anyNumber(). Can you explain what do yo mean with "some doubts about architecture"?

@cakeinpanic cakeinpanic force-pushed the not branch 2 times, most recently from 246e120 to 494ebe5 Compare January 27, 2018 19:49
@cakeinpanic
Copy link
Contributor Author

@NagRock
I don't like that if you need to add one more matcher, you need to copypaste almost the same code to ts-mockito.ts and Not.ts

But it doesn't matter when we speak about how it will be used in the end)

I've fixed everything, take a look please

@cakeinpanic cakeinpanic force-pushed the not branch 2 times, most recently from 19bcd48 to 32bd8d8 Compare January 29, 2018 16:30
@cakeinpanic
Copy link
Contributor Author

@NagRock hello! can you take a look at PR? Any comments?

README.md Outdated
verify(mockedFoo.getBar(not().strictEqual(2))).once(); // was called with anything except 2 once
verify(mockedFoo.getBar(between(2, 3))).thrice(); // was called with arg beween 2-3 exactly three times
verify(mockedFoo.getBar(anyNumber()).times(4); // was called with any number arg exactly four times
verify(mockedFoo.getBar(not().anyNumber).times(4); // was called with anything but not a number exactly four times
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing () on anyNumber -> should be: not().anyNumber()

@@ -114,14 +115,18 @@ export function strictEqual(expectedValue: any): Matcher {
return new StrictEqualMatcher(expectedValue);
}

export function match(expectedValue: RegExp | string): Matcher {
return new MatchingStringMatcher(expectedValue);
export function matchString(expectedValue: RegExp | string): Matcher {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Version with match name has been already released so we should not change it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or we can add matchString as duplicated function and add deprecation infromation to match.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets leave it as match

@@ -1,38 +1,45 @@
import {Matcher} from "../../../src/matcher/type/Matcher";
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about new spec file for Not matcher? Now spec descriptions are a little bit strange checking if class matches returns true but in then section we got expect(notResult).toBeFalsy();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a question!
by adding notResult check to every unit I wanted to notify every contributor that he should provide his matcher to Not, otherwise it won't work(

This is the _lack of architecture` I've mentioned before

maybe just rewrite texts in it?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed it, yake a look please

@cakeinpanic
Copy link
Contributor Author

@bendykowski take a look at this PR too please

@bendykowski
Copy link
Collaborator

@cakeinpanic I'll try to check it today evening when I get home

@bendykowski
Copy link
Collaborator

@cakeinpanic I've checked the PR and found out a bit cleaner implementation but didn't have time to finish it. I'll do it today afternoon.

@cakeinpanic
Copy link
Contributor Author

@bendykowski opened new PR extending this one #84

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants